-
Notifications
You must be signed in to change notification settings - Fork 855
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Return errors from pcap_dump and pcap_dump_close #1048
base: master
Are you sure you want to change the base?
Conversation
821e1c1
to
b7b52e3
Compare
Thank you for proposing these changes. Regardless of the more substantial aspects, which also need to be reviwed, it would be difficult to remember the difference between |
+1
|
I don't agree, the action/verb is the same. You wouldn't add _with_error on
every function that returns an error of it wasn't to fix a API issue. I
marked pcap_dump_close as deprecated, the intention is to use only the new
version in new code. The 1 is just there as a reminder that a mistake in
ABI/API had to be fixed. Linux kernel takes the same approach with
syscalls, Microsoft appends "ex" to their functions, libs like zstd and
others appends version number to the replacement functions.
If you insist i will of course change it to whatever name you want.
I've tested this code with my custom capture tool and it works well.
…On Wed, Aug 25, 2021, 23:32 Denis Ovsienko ***@***.***> wrote:
Thank you for proposing these changes. Regardless of the more substantial
aspects, which also need to be reviwed, it would be difficult to remember
the difference between pcap_dump1() and pcap_dump() (or pcap_dump2() if
there is another variation in future). It would be easier with a name like
pcap_dump_reliably(), pcap_dump_or_fail() or some such.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#1048 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AABLO26SEGEY7JMKHYGVQA3T6VOM5ANCNFSM5CZFVGOQ>
.
|
What's the conclusion on the naming of these new functions? I would like to
move forward and discuss the implementation changes and hopefully get this
merged.
…On Thu, Aug 26, 2021 at 12:23 AM Erik Rigtorp ***@***.***> wrote:
I don't agree, the action/verb is the same. You wouldn't add _with_error
on every function that returns an error of it wasn't to fix a API issue. I
marked pcap_dump_close as deprecated, the intention is to use only the new
version in new code. The 1 is just there as a reminder that a mistake in
ABI/API had to be fixed. Linux kernel takes the same approach with
syscalls, Microsoft appends "ex" to their functions, libs like zstd and
others appends version number to the replacement functions.
If you insist i will of course change it to whatever name you want.
I've tested this code with my custom capture tool and it works well.
On Wed, Aug 25, 2021, 23:32 Denis Ovsienko ***@***.***>
wrote:
> Thank you for proposing these changes. Regardless of the more substantial
> aspects, which also need to be reviwed, it would be difficult to remember
> the difference between pcap_dump1() and pcap_dump() (or pcap_dump2() if
> there is another variation in future). It would be easier with a name like
> pcap_dump_reliably(), pcap_dump_or_fail() or some such.
>
> —
> You are receiving this because you authored the thread.
> Reply to this email directly, view it on GitHub
> <#1048 (comment)>,
> or unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AABLO26SEGEY7JMKHYGVQA3T6VOM5ANCNFSM5CZFVGOQ>
> .
>
|
@guyharris @infrastation any updates regarding the naming? |
@guyharris @infrastation reminding you about this again. |
Thank you for waiting. Naming consistency within a project usually works better than naming consistency between different projects, so it seems likely that before the new function names get set in stone they should be made to make the most sense to libpcap API users. A good usage example for the new functions would be in tcpdump source code, this would also make it easier to confirm everything fits as expected. I have made a look at The first instance of The second instance looks similar. The first instance of The second instance of The above changes in tcpdump would need to be conditional depending on |
On Thu, Sep 16, 2021 at 4:24 PM Denis Ovsienko ***@***.***> wrote:
Thank you for waiting. Naming consistency within a project usually works
better than naming consistency between different projects, so it seems
likely that before the new function names get set in stone they should be
made to make the most sense to libpcap API users.
Any name is fine with me, I only care about the functionality.
The first instance of pcap_dump_close() closes the file before opening
the next one. Assuming a single file error should not abort the sequence,
the only use for checking the return value of pcap_dump_close_GHPR1048()
would be to skip the file compression and to print a warning message. In
which case it would be useful to explain the error condition. Which makes
me think it might be useful to mention in the man pages that when a new
function returns PCAP_ERROR, the calling function can use errno to work
the cause out.
errno is going to be EIO, ENOSPC, EDQUOT. Opening the next file will likely
fail, but at least logging that the disk is full would be great.
The first instance of pcap_dump() knows it is writing to a file in a
sequence, but it does not know how many more packets the current file is
going to have, so the above thinking likely does not stand and the only
reasonable thing to do on the first failed packet would be to fail right
away with an error message. Which, again, would benefit from a meaningful
error cause.
This is actually the more important case because if the disk is full,
tcpdump will keep on calling pcap_dump() and that data will be silently
discarded. If someone then makes more disk space available, writing will
start to succeed again. The resulting file is likely to be corrupted
because a header + data before the failure was only partially written.
The above changes in tcpdump would need to be conditional depending on
HAVE_PCAP_DUMP_CLOSE_GHPR1048 and HAVE_PCAP_DUMP_GHPR1048 or some such,
so tcpdump continues to compile with older libpcap versions. Thus the
complication is not only in the proposed changes itself, but also in the
incurred changes. So it would be best not to rush this before verifying
everything.
My capture tool uses the new API to detect write errors and will log and
then exit with error code. Systemd will then try to continually restart the
capture process. This way I won't get corrupted pcap files saved. Our
monitoring will also alert us that a process is failing, but that's kind of
useless since ops would have missed the disk is full alerts, oh well.
|
Yeah, but names accumulate, get in the way, and confuse people. |
pcap_dump.3pcap
Outdated
@@ -30,6 +30,9 @@ pcap_dump \- write a packet to a capture file | |||
void pcap_dump(u_char *user, struct pcap_pkthdr *h, | |||
.ti +8 | |||
u_char *sp); | |||
int pcap_dump(u_char *user, struct pcap_pkthdr *h, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pcap_dump1
Now that pull request #494 has been merged, this change needs to be rebased. |
361e738
to
5158776
Compare
@infrastation I fixed the manpage and rebased! |
5158776
to
cecef30
Compare
Closes #1047